Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct reading of sync-labels input. #480

Merged
merged 3 commits into from
Mar 23, 2023

Conversation

adam-azarchs
Copy link
Contributor

@adam-azarchs adam-azarchs commented Jan 11, 2023

Description:
Correctly check the truth value of the sync-labels input.
Contrary to the assumptions made in the unit tests, core.getInput always returns a string, and !!'false' is true.

Related issue:
Fixes #112

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

BREAKING CHANGE: previously, any non-empty string would result in labels being deleted. With this change, an empty string will cause an error, but strings representing yaml-compliant boolean values will be correctly interpreted.

@adam-azarchs adam-azarchs requested a review from a team as a code owner January 11, 2023 23:02
@MaksimZhukov
Copy link
Contributor

Hello @adam-azarchs ! Thank you for the contribution!
Your PR is a duplicate of this one. Unfortunately, there is no response from the author.
Could you apply the suggested changes.

@adam-azarchs
Copy link
Contributor Author

Could you apply the suggested changes.

As my comment makes clear, I didn't want to do that because doing so would cause immediate breakage for everyone who was using the workaround of setting sync-labels: ''.

@adam-azarchs
Copy link
Contributor Author

(granted it's still a breaking change, but only for people who were setting sync-labels to something other than a true/false/empty string, and the behavior for those people was probably not what they'd expected anyway)

@MaksimZhukov
Copy link
Contributor

@adam-azarchs thank you for the explanation!
As you mentioned, your PR introduces a breaking change either way, so we will release a new major version of the action. Everyone who uses the current version will not be broken, so there is no need for backward compatibility.

@adam-azarchs
Copy link
Contributor Author

Thinking more about it, I agree, since the failure mode will be the action failing loudly, as opposed to just not doing what the user wanted. I'll make that change.

Copy link
Contributor

@MaksimZhukov MaksimZhukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adam-azarchs thank you for resolving the comment, I left a few more, please take a look.

src/labeler.ts Outdated Show resolved Hide resolved
__tests__/main.test.ts Outdated Show resolved Hide resolved
Contrary to the assumptions made in the unit tests, core.getInput
always returns a string, and !!'false' is true.

Also updates the unit test to reduce changes of regressions by ensuring
that the mocked getInput returns a string, as it would in production.

Fixes actions#112

Make sure test catches regressions.
@adam-azarchs
Copy link
Contributor Author

Thanks for the review so far! I believe I've addressed all of the feedback at this point; please let me know if there is anything further to do (I'm not in a rush to merge this or anything, but I want to know if I can cross it off my to-do list...)

@MaksimZhukov
Copy link
Contributor

Hello @adam-azarchs! Thank you!
I will ask my team members to review this PR as well.
We will merge it when the changes from this PR are ready. We would like to include them in one release.

@dfandrich
Copy link
Contributor

There are 23 other open PRs right now. Are any of them planned to get into the next release as well?

@MaksimZhukov
Copy link
Contributor

Hello @dfandrich !
We are currently thoroughly investigating the issues and pull requests in the repository, so most likely some changes will be also included or released separately

@dfandrich
Copy link
Contributor

dfandrich commented Jan 30, 2023 via email

@ghusse
Copy link

ghusse commented Mar 17, 2023

Is there any news regarding the release of this fix? The bug is quite annoying.

@MaksimZhukov
Copy link
Contributor

Hello @ghusse! We apologize for the inconvenience!
We will merge it when the changes from this PR are ready. We would like to include them in one major release.

@MaksimZhukov
Copy link
Contributor

Hello @adam-azarchs! Could you please resolve the conflicts? We would like to merge the PR

@MaksimZhukov MaksimZhukov merged commit 751921c into actions:main Mar 23, 2023
@jjerphan
Copy link

Thank you all for fixing #112!

MaksimZhukov added a commit that referenced this pull request May 23, 2023
@kachkaev kachkaev mentioned this pull request May 31, 2023
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Jan 2, 2024
The issue[0] for this workaround has been resolved[0], so we can set it
to a proper boolean field.

[0] systemd#18671
[1] actions/labeler#480
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Jan 2, 2024
The issue[0] behind this workaround has been resolved[1], so we can set it
to a proper boolean field.

[0] systemd#18671
[1] actions/labeler#480
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Jan 2, 2024
The issue[0] behind this workaround has been resolved[1], so we can set it
to a proper boolean field.

[0] systemd#18671
[1] actions/labeler#480
mrc0mmand added a commit to systemd/systemd that referenced this pull request Jan 2, 2024
The issue[0] behind this workaround has been resolved[1], so we can set it
to a proper boolean field.

[0] #18671
[1] actions/labeler#480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to only add label, not remove when PR is opened?
7 participants